Adding support for @implements. - #36292

Yes I would like to try it out. I will be able to do so next week when I am back in the office.

To be able to debug, I would need a bot-pack build that I can try out. Let me know when you have one available 😄

Tim van der Lippe (@TimvdLippe) I unfortunately can't summon the TS bot .. let me try something, <clears-throat />

<incantation-voice>
Oh mighty and benevolent TS overlords Nathan Shively-Sanders (@sandersn) Orta Therox (@orta) or Daniel Rosenwasser (@DanielRosenwasser) could we have a pack of this ?
</incantation-voice>

Sure!
TypeScript Bot (@typescript-bot) pack this

Heya Orta Therox (@orta), I've started to run the tarball bundle task on this PR at 5a7fdd5. You can monitor the build here. It should now contribute to this PR's status checks.

Hey Orta Therox (@orta), I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/61139/artifacts?artifactName=tgz&fileId=9B83D1D6A5EF666A0539602791C1631D23484EBBA1F4A0A5D118C77653B5832102&fileName=/typescript-3.8.0-insiders.20200121.tgz"
    }
}

and then running npm install.

There is also a playground for this build.

Titian Cernicova-Dragomir (@dragomirtitian) re your questions:

1. Suppressing errors that can't be fixed in JS is a good idea. We can re-instate them later if we come up with a way to fix them.
2. I believe the code you emit for the code fix would have to be different, at least in that it uses jsdoc instead of type annotations. And it may have to fail because of errors from (1). That is a lot of work so you can open a new issue instead and assign it to me (or ping me on it).

I will look at the code now.

I'm a linguist, so I prefer the traditional 3 invocations. Like this: NoamChomsky NoamChomsky NoamChomsky

I can confirm that the updated build is catching issues like missing overrides. That said, we are hitting a couple of other issues with TypeScript (#19983) which makes it hard for us to fully test all of its internals. My understanding thus far is that this change is WAI.

One improvement that would be great is that methods with @return on an @interface do not result in the A function whose declared type is neither 'void' nor 'any' must return a value. error. At that point, I don't see any TypeScript blockers for supporting JSDoc interfaces (without special-casing for TypeScript).

Thank you for doing the work and for the quick implementation!

Tim van der Lippe (@TimvdLippe) The scope of this was specifically defined as support for @implements but I saw the same issue as you with regard to the actual usability of this with functions using @return, and I mentioned it in my opening comment to this pr. If Nathan Shively-Sanders (@sandersn) gives me the go ahead to suppress the error for classes marked with @interface I can roll up that chage in this pr, no problem (would probably have it done by Monday).

If not I think the only workaround for now would be to @ts-ignore it, although I know that is not really a viable solution for this existing code base...

Titian Cernicova-Dragomir (@dragomirtitian) I'd like to keep support for @interface in a separate PR. Anyway, I'm not keen on the idea of ignoring errors on what would be invalid Typescript.
Tim van der Lippe (@TimvdLippe) Is // @ts-nocheck at the beginning of the interface file a solution? I assume that it will be replaced by a d.ts once you are further along in the migration.

Nathan Shively-Sanders (@sandersn) Given our extensive use of @interface, a no-check would probably not fly. Especially as the interfaces and usages are scattered throughout the codebase. I suppose adding ts-ignore for every single method is the least intrusive (interim) solution.

I am okay with punting that to a different PR though.

Looking at the example in the jsdoc documentation (excerpted below), I note that throwing in the interface body satisfies Typescript because throwing causes the function to return never, which is assignable to anything:

/**
 * @interface
 */
function Color() {}

/**
 * @returns {Array<number>} An array containing the red, green, and blue values, in that order.
 */
Color.prototype.rgb = function() {
    throw new Error('not implemented');
};

Does that work with Closure?

Nathan Shively-Sanders (@sandersn) I just checked and the following does indeed compile in both Closure and TS (with this patch):

/**
 * @interface
 */
export class EventTarget {
  /**
   * @param {symbol} eventType
   * @param {function(!Common.Event)} listener
   * @param {!Object=} thisObject
   * @return {!Common.EventTarget.EventDescriptor}
   */
  addEventListener(eventType, listener, thisObject) {
    throw new Error(`Not implemented`);
  }
}

Nathan Shively-Sanders (@sandersn) I fixed the code review issues. I added a test for multiple interfaces. I ran into the following issues;

1. I am not sure exactly how to suppress the error for missing signature implementation in JS. I mean I know how the error is produced (the error is deep in the bowels of checkTypeRelatedTo which is called when checking the heritage clauses). My issue is I am unsure how to suppress errors coming from there. I could add a parameter to checkTypeRelatedTo to skip index checks during implements checks, but that seems very ad hoc. The other option might be to capture diagnostics using errorOutputContainer but I would have to see if that could work. If you can think of an example similar to this, I could use the 'inspiration' to make sure I don't go off the rails 😂.

2. You mentioned four-slash tests for type completions in @implements which would be similar to @extends. The issue is but I could not trigger it completions for @augments and @extends at all I would create a separate issue to fix both. Thoughts?

Also sorry for the delay, it's been a hectic week at work.

1. I couldn't think of a way to suppress errors that was both easy and obviously correct. Plus, after more thought, I'm not sure it's the right thing. @implements is essentially just a check, which means that people will only use it get errors, not to provide better intellisense. (Disregarding the completions that don't work and should.) For people who care mainly about correctness, having JS behave differently from TS is only going to confuse them.
2. Definitely create a separate issue for completions.

TypeScript Bot (@typescript-bot) user test this

Heya Nathan Shively-Sanders (@sandersn), I've started to run the parallelized community code test suite on this PR at d7839fe. You can monitor the build here. It should now contribute to this PR's status checks.

ha ha oh no now user tests are broken because of the new octokit.

(fix is at #36915; I'll merge it after I see that it can post a user test PR)

Titian Cernicova-Dragomir (@dragomirtitian) can you merge from master and then I'll run the user tests?

Nathan Shively-Sanders (@sandersn) Yes, I'll merge from master. First I want to fix the declaration emit for @interface since it is not being emitted right now. (Forgot about declaration emit in JS)

Nathan Shively-Sanders (@sandersn) I merged master, and fixed declaration emit. You should have a look at the emit fixes.

TypeScript Bot (@typescript-bot) user test this

Heya Nathan Shively-Sanders (@sandersn), I've started to run the parallelized community code test suite on this PR at cedc792. You can monitor the build here. It should now contribute to this PR's status checks.

Nathan Shively-Sanders (@sandersn) The user tests seem to have failed again :(

I think this will be ready once you get rid of the unneeded checks.

Nathan Shively-Sanders (@sandersn) Ok, I think we are good to go!

Titian Cernicova-Dragomir (@dragomirtitian) Thanks for your hard work on this one!